-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Parse all SPARQL Update Requests #1574
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1574 +/- ##
==========================================
- Coverage 89.06% 89.04% -0.02%
==========================================
Files 368 368
Lines 34039 34094 +55
Branches 3846 3847 +1
==========================================
+ Hits 30316 30360 +44
- Misses 2471 2481 +10
- Partials 1252 1253 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A first pass over the code (not the tests)
This is already looking code, I only have minor comments and questions.
Keep up the good work!
auto checkTriples = | ||
[&isVisibleIfVariable, | ||
&ctx](const std::vector<SparqlTripleSimpleWithGraph>& triples) { | ||
for (auto& triple : triples) { | ||
if (!(isVisibleIfVariable(triple.s_) && | ||
isVisibleIfVariable(triple.p_) && | ||
isVisibleIfVariable(triple.o_))) { | ||
reportError(ctx, | ||
absl::StrCat("A triple contains a variable that was " | ||
"not bound in the query body.")); | ||
} | ||
} | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this duplicated/copy-pasted from some other type of query?
If so, remove that duplication by helper functions. Else I am going to check this some other time when I am less tired, this seems to be the most complicated rule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK this is not duplicated. But maybe a (currently private) of ParsedQuery
can be used instead of isVisibleIfVariable
.
}); | ||
std::ranges::move(visitVector(ctx->quadsNotTriples()), | ||
std::back_inserter(triplesWithGraph)); | ||
return ad_utility::flatten(std::move(triplesWithGraph)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are reordering the triples here. Is that correct, because the order has no semantics? Then write a short comment to make it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To my understanding the order has no meaning here, no. In the end we get a set of quads that is added to/removed from one or multiple graphs. The "right" order can also not be reconstructed with the current setup. The quads <a> <b> <c> GRAPH <foo> { <d> <e> <f> }
and GRAPH <foo> { <d> <e> <f> } . <a> <b> <c>
can not be distinguished in their parsed form (meaning in the visitor).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few minor comments for the test design.
Consider merging in the current master branch to make the coverage report more consistent.
Conformance check passed ✅No test result changes. |
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much!
The SPARQL parser now supports all kinds of
UPDATE
s. These are parsed into a unified classUpdateClause
which is part of theParsedQuery
. This is another major step towards support for SPARQL UPDATE.